Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for updating and deleting MapData features #2112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rwrx
Copy link
Contributor

@rwrx rwrx commented Nov 29, 2019

Add support for updating and deleting MapData features. I have only created updating and deleting polyline features as I am not sure if you would want to add this functionality. If you would want it, I can make it more general and clean it up, because now it is only a very ugly hack.

@westnordost
Copy link
Contributor

westnordost commented Jan 18, 2020

That is exactly what I need. Currently I re-set the whole MapData whenever one geometry is added or removed. I can imagine this is not very performant but there doesn't seem to be another option to do this right now.

@matteblair
Copy link
Member

This does save some processing compared to re-setting all of the features in the layer, but since any feature change needs to propagate down to GPU buffers, we still need to rebuild those buffers and upload them to the GPU again.

It's not obvious that this feature would make enough difference in performance to justify the added complexity. Have you done any profiling to see where the bottlenecks are?

@westnordost
Copy link
Contributor

Would love to see a comparison in terms of speed.

What this saves in any case is the necessity to keep the data in Java as well (to reach the same functionality). So, apart from any considerations of speed, it'd be a more convenient interface.

@rwrx
Copy link
Contributor Author

rwrx commented Jan 23, 2020

@matteblair Unfortunately I have no experience with profiling C++ native code on Android. Do you have some hints how can I profile it?

@westnordost
Copy link
Contributor

westnordost commented Jan 24, 2020

Couldn't you just count the milliseconds it takes for

  1. A: set 10000 map features on a map data layer
  2. B: add 1 feature to that map data layer

to complete and print it to console?

B then is the time it it takes for rebuilding the buffers and upload them to the GPU.
A-B is the time it takes to copy the data from Java, the time saved by this implementation.

@matteblair
Copy link
Member

There's also work that happens asynchronously when map data is changed, that would have to be instrumented differently. But the measurement you described could be informative.

@rwrx
Copy link
Contributor Author

rwrx commented Feb 4, 2020

Ok, I have profiled methods MapData.addFeature(Geometry feature) and my added method MapData.updatePolyline(long id, Map<String, String> properties) by adding and 586 polylines and on average MapData.updatePolyline(long id, Map<String, String> properties) was faster about 15%.

Both profiled methods also calls a C++ method ClientDataSource::generateTiles().

@westnordost
Copy link
Contributor

Can you also say the absolute numbers?

Note on ClientDataSource::generateTiles(): Could this be optimized? After all, if you add just one feature, not all tiles need to be (re-)generated, but only those that contain the one feature updated.

@matteblair
Copy link
Member

I'm interested in absolute numbers too - if makes a difference if the change is a few microseconds vs a few milliseconds.

The main operations in generateTiles() happen inside geojson-vt-cpp, a 3rd-party library. It's conceivable that we could modify its behavior to optimize our use cases, but I'd rather not diverge from the source if possible.

@rwrx
Copy link
Contributor Author

rwrx commented Feb 4, 2020

Ok, profiling scenario was this: add polyline by MapData.addFeature then update polyline with MapData.updatePolyline and this repeated for 586 polylines.

For first couple of polylines it was under 10 milliseconds for both both methods. And then it started to slow down. At the end it was around 150 miliseconds for MapData.updatePolyline and around 170 miliseconds for MapData.addFeature.

I suggest that you can also profile it, so we will have better understanding of actual performance difference.

Overall in average there was 15% difference.

@westnordost
Copy link
Contributor

So, the big majority of the time is not spent in copying the data from Java to native but in generating the tiles (or uploading to buffer). So from a sheer performance point of view, this (alone) does not make much of a difference, but what it does it to have a more convenient interface. And, it opens up the path for more performance improvement (on geojson-vt-cpp).

@tallytalwar
Copy link
Member

When we first introduced ClientDataSources, we had support for updating MapData features, but performance bottleneck on geojson-vt library was present then also. I vaguely remember having similar discussions during good old eraser-map (ClientDataSource was implemented to fulfill the needs for Erasermap app) development days at mapzen, which is when we decided to remove this feature.

Base automatically changed from master to main February 15, 2021 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants